Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass pacakge names internally as symbols #658

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

DarwinAwardWinner
Copy link
Collaborator

The function "el-get-as-symbol" is only used in interactive functions
that accept user input. All other functions assume packages are passed
and returned as symbols.

Assertions have been added to many functions that take packages or
lists of packages to enforce this. These can be removed later once we are sure that we're not passing any strings internally.

Assertions have been added to many functions that take packages or
lists of packages to enforce this.

by itself, this breaks el-get horribly, because other functions must
also be modified not to pass strings when they call these functions.
The function "el-get-as-symbol" is only used in interactive functions
that accept user input. All other functions assume packages are passed
and returned as symbols.
@dimitri
Copy link
Owner

dimitri commented Mar 10, 2012

Well I guess that if you run the test suite against the assert-enabled branch that would allow us ascertain we're only using symbols internally. Do we need to include the asserts in the official el-get code (not opposed to it per-se, only curious).

@DarwinAwardWinner
Copy link
Collaborator Author

No, we don't. That's why I put them in a separate commit, so I could easily revert them later. I've run the test suite against this branch, and all the tests passed.

@dimitri
Copy link
Owner

dimitri commented Mar 10, 2012

I'm not convinced we should install the asserts, however, please do as you see fit. Maybe with a new function el-get-verbose-assert so that we only assert when el-get-verbose?

@DarwinAwardWinner
Copy link
Collaborator Author

No, we shouldn't install the asserts. They're just for testing purposes, so that any package-names passed as strings will get caught quickly during testing with a predictable error message.

@dimitri
Copy link
Owner

dimitri commented Mar 10, 2012

As you see fit, and thanks agin for all the testing :)

@dimitri
Copy link
Owner

dimitri commented Dec 14, 2012

Where are we on that? Is it still needed?

@DarwinAwardWinner
Copy link
Collaborator Author

This isn't high-priority, because it isn't actually fixing any bugs. This can wait until after the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants